Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add home page structure to Help Site #10117

Merged
merged 26 commits into from
Aug 9, 2022
Merged

add home page structure to Help Site #10117

merged 26 commits into from
Aug 9, 2022

Conversation

marcochavezf
Copy link
Contributor

@marcochavezf marcochavezf commented Jul 26, 2022 •
edited
Loading

cc @michelle-thompson

Details

This PR only includes the structure for the homepage and fixes the styles for the homepage and hub pages on large screens. The site's content is still WIP, but once the content is done, we're just going to replace the text in the respective HTML or Markdown files and can be done by someone of the resource management team.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/216731

Tests

  1. (you need ruby and bundler installed to do this, more info here)

  2. Run:

    cd docs
bundle install
bundle exec jekyll serve

  3. Navigate to http://localhost:4000/main and verify the structure of the homepage is displayed as in the design document mockups or the screenshots attached at the end.

QA Steps

Go to https://help.expensify.com/main and verify the structure of the homepage is displayed (as in the screenshots)

Screenshots

Web

localhost_4000_main

Big screen:
Screen Shot 2022-07-26 at 14 38 20

Mobile Web

Screen Shot 2022-07-26 at 14 38 20

@marcochavezf marcochavezf requested a review from a team as a code owner July 26, 2022 19:42
@marcochavezf marcochavezf self-assigned this Jul 26, 2022
@melvin-bot melvin-bot bot requested review from tgolen and removed request for a team July 26, 2022 19:42
@michelle-thompson
Copy link
Contributor

The content on the home screen isn't showing up - is that to be expected?

I noticed that on hover, links are underlined. I think we should stick to similar styling as UseDot, so let's remove the underlining for hover.

@marcochavezf
Copy link
Contributor Author

The content on the home screen isn't showing up - is that to be expected?

Oh, do you mean just an empty screen? Like this one:

Screen Shot 2022-07-26 at 15 00 01

Because currently the Homepage (/main) is empty in https://help.expensify.com/main, and won't show up until this PR is merged (we can only see the changes locally).

I noticed that on hover, links are underlined. I think we should stick to similar styling as UseDot, so let's remove the underlining for hover.

Sounds good! Removing the underling for hover...

@michelle-thompson
Copy link
Contributor

Okay, I can't run locally but I'm curious about the hover over the card - what does that look like?

And one more thing I noticed on mobile - the bolded body text doesn't look like GT America
IMG_EF7444D43CDF-1

@marcochavezf
Copy link
Contributor Author

marcochavezf commented Jul 26, 2022 •
edited
Loading

The underling for hover was removed and looks like this:

Screen.Recording.2022-07-26.at.15.08.43.mov

Okay, I can't run locally but I'm curious about the hover over the card - what does that look like?

This is what the hover over a card looks like, it just changes the cursor to pointer:

Screen.Recording.2022-07-26.at.15.11.33.mov

I'm wondering if we add an animation or just leave it as it is, what do you think?

And one more thing I noticed on mobile - the bolded body text doesn't look like GT America

Yeah, seems we had a bad font configuration for the bolded text, this is how it looks using the default configuration (the difference is subtle but the bolded text looks a bit smaller):

Screen Shot 2022-07-26 at 15 31 05

@michelle-thompson
Copy link
Contributor

For the cards, how about we change the border color upon hover? So for the blue, green, and grey borders, they all darken by 2.5% - I found this in UseDot, I'm assuming that's what it means, hahah.
image

@michelle-thompson
Copy link
Contributor

As for the mobile font - that looks like medium weight, can we change to bold?

@marcochavezf
Copy link
Contributor Author

marcochavezf commented Jul 27, 2022 •
edited
Loading

Hi @michelle-thompson, I was playing with the border color upon hover, and I think a good value to notice the color change is 15%, because for 2.5% the difference is hardly noticeable imo, what do you think?

Screen.Recording.2022-07-27.at.12.35.49.mov

This is how it looks with 2.5%:

Screen.Recording.2022-07-27.at.12.36.18.mov

As for the mobile font - that looks like medium weight, can we change to bold?

We had another bad configuration hehe. For the bold text we were using font-weight: 600, but now I changed it to font-weight: bold and this is how it looks:

Mobile:
Screen Shot 2022-07-27 at 12 51 36

Desktop:
localhost_4000_articles_request-money_SmartScan

@michelle-thompson
Copy link
Contributor

@marcochavezf 15% and bold weight look great 👍

@marcochavezf
Copy link
Contributor Author

hi @tgolen! the visual changes are done, the code is ready to be reviewed 👍🏽

tgolen
tgolen requested changes Aug 2, 2022
View reviewed changes
docs/main.html Outdated Show resolved Hide resolved
@marcochavezf
Copy link
Contributor Author

I updated the code to use anchors instead of clickable divs, it's ready to be reviewed again!

tgolen
tgolen previously requested changes Aug 3, 2022
View reviewed changes
docs/_includes/home-lhn.html Show resolved Hide resolved
docs/_includes/home-lhn.html Outdated Show resolved Hide resolved
@marcochavezf
Copy link
Contributor Author

Updated the remaining clickable divs in the code, it's ready to be reviewed again 😀

@marcochavezf marcochavezf dismissed tgolen’s stale review August 5, 2022 19:12

changes already addressed

@marcochavezf
Copy link
Contributor Author

Hi @tgolen! friendly bump since we have other issues that depend on these changes :) Thanks!

tgolen
tgolen previously requested changes Aug 9, 2022
View reviewed changes
docs/assets/js/main.js Outdated Show resolved Hide resolved
@marcochavezf marcochavezf dismissed tgolen’s stale review August 9, 2022 20:21

moved event listeners to DOMContentLoaded

@marcochavezf
Copy link
Contributor Author

Updated with the DOMContentLoaded event, ready to be reviewed again @tgolen!

tgolen
tgolen approved these changes Aug 9, 2022
View reviewed changes
Copy link
Contributor

@tgolen tgolen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@tgolen tgolen merged commit 4d6558c into main Aug 9, 2022
@tgolen tgolen deleted the marco-addHomePage branch August 9, 2022 22:17
@OSBotify
Copy link
Contributor

OSBotify commented Aug 9, 2022

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify OSBotify mentioned this pull request Aug 15, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgolen tgolen tgolen approved these changes

Assignees

@marcochavezf marcochavezf

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@marcochavezf @michelle-thompson @OSBotify @tgolen